better error handling when working with kube and kudo. #1097
Conversation
|
Oh... I meant to mention that I was looking to get rid of "github.com/pkg/errors" :) |
| return nil, errors.WithMessage(err, "operators") | ||
| // timeout is not a wrappable error, timeout is an underlying issue that is NOT CRD specific, there is no value in wrapping or converting as well. | ||
| // best to provide the actual error for proper reporting. | ||
| if os.IsTimeout(err) { |
There was a problem hiding this comment.
I understand that we don't wrap the timeout error but to what end? Where do you do anything with it?
There was a problem hiding this comment.
The intention is to do something with it :). I was wrongly of the opinion that this was such a simple and not debatable change that I could land this quickly and move on to using it.
| if os.IsTimeout(err) { | ||
| return nil, err | ||
| } | ||
| return nil, fmt.Errorf("operators crd: %w", err) |
There was a problem hiding this comment.
Since you're at it: I'm not sure what "operators crd: adds to the error message. Maybe smth. like:
| return nil, fmt.Errorf("operators crd: %w", err) | |
| return nil, fmt.Errorf("failed to fetch operators.kudo.dev CRD: %w", err) |
?
There was a problem hiding this comment.
this request for change is not consistent with the actual change. The message that you are wanting to change was original there. Lets focus on the change.
yeah, I struggled with that as well and I would like to understand, could you maybe explain @kensipe ? and how it connects to the issue linked as well. I don't have anything against the change per say but I don't understand the motivation... |
|
@zen-dog This change was made because the previous code wrapped the error with the GH Making matters worse... in go core there are 2 types of timeouts... what around context... another based on an internal error. the common solution for detecting if it is a timeout is to use It was proposed to fix in 1.13. Ross Cox indicates in golang/go#31449 (comment) that it is targeted for 1.14. The best summary is: golang/go#30322 For more reading |
What this PR does / why we need it:
We can NOT wrap the timeout error associated with a bad kubeconfig. It is private error which can be interrogated to determine that it was a
Timeouthowever that ability doesn't work if wrapped.When the cluster is down or not started DNS requests from go will fail on Line 142 of https://golang.org/src/net/dial.go with an error which is private (internal). It can not be wrapped because it can't be unwrapped.
Fixes #
This part 1 of a 2 part solution towards fixing bug #1056